-
Notifications
You must be signed in to change notification settings - Fork 346
Scaled UI Jettons #526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Scaled UI Jettons #526
Conversation
text/0000-scaled-ui-jettons.md
Outdated
|
|
||
| `onchain_balance` - (integer) - the balance of a jetton wallet or a transfer amount as reported by `get_wallet_data()` call on the jetton wallet or in onchain data | ||
|
|
||
| `displayed_balance` - (integer) - the equivalent amount that should be displayed in UIs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds imprecise; after all, not even nanoTON amount is displayed in UIs as is. Maybe do a [int userfacing_numerator, int userfacing_denominator], or a fixed-point fraction (with denominator of either 2^64, 2^128 or 10^18)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
muldiv internally uses 513 bits for calculation instead of 257. And standard is not limited to this. Arbitrary implementation may include numerator/denominator. The key point here is that we delegate this logic to the contract implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text/0000-scaled-ui-jettons.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ProgramCrafter I have added a remark to hopefully resolve the confusion around the usage of decimals.
|
I see two problems in this TEP.
|
|
This is elegantly solved by requiring custom payload to contain the active multiplier. This solution also has the side benefit of supporting true rebase jettons, but its drawback is that it starts prying into custom payload contents. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text/0000-scaled-ui-jettons.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text/0000-scaled-ui-jettons.md
Outdated
|
|
||
| `onchain_balance` - (integer) - the balance of a jetton wallet or a transfer amount as reported by `get_wallet_data()` call on the jetton wallet or in onchain data | ||
|
|
||
| `displayed_balance` - (integer) - the equivalent amount that should be displayed in UIs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text/0000-scaled-ui-jettons.md
text/0000-scaled-ui-jettons.md
Outdated
| They MUST additionally (w.r.t. TEP-74) support the following get methods: | ||
| 1. `get_supply_data()` returns `(int total_onchain_supply, int total_displayed_supply)` | ||
|
|
||
| `total_onchain_supply` - (integer) - the sum of balances of jetton wallets of this jetton master (as reported by `get_wallet_data()` calls on the jetton wallets) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be equal to the total_supply field from the get_jetton_data, shouldn't it? If so I propose to return only the total_displayed_supply here and reuse existing get method. Otherwise developers could create implementation which returns different values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should, and I will rewrite the text to say so. However I think returning both values at the same time is necessary so as to avoid a situation where one get method returns a value from state A and the next call to another get method returns a value from state B, resulting in inconsistent calculations.
I guess indexers could apply the same approach to this as with regular jetton balances - index latest multiplier change on master contract (if we accept this TEP then we can detect it) and store it for corresponding jetton. With this, retrieving balance for user will still be one select, without the need to run tvm call every time. |
…tandard. Add an external-out message to report supply data changes.
|
@mr-tron @Kaladin13 I have changed the TEP to hopefully allow for a simpler indexer implementation |
|
What about existing Jetton contracts? Should we consider implementing a workaround to give them the same level of visibility in the interfaces? |
In a similar way to TEP-89? (quote from TEP-89) |
text/0000-scaled-ui-jettons.md
Outdated
|
|
||
| Jetton master contracts supporting this TEP MUST send the following external-out message (TL-B structure) whenever the values returned by `get_display_multiplier()`: | ||
| ``` | ||
| display_multiplier_changed#ac392598 numerator:(VarUInteger 32) denominator:(VarUInteger 32) {n:#} comment:(Maybe (SnakeData ~n)) = ExternalOutMsgBody; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong scheme. should be
display_multiplier_changed#ac392598 numerator:(VarUInteger 32) denominator:(VarUInteger 32) {n:#} comment:(Maybe (SnakeData n)) = ExternalOutMsgBody n;
or better
display_multiplier_changed#ac392598 numerator:(VarUInteger 32) denominator:(VarUInteger 32) comment:(Maybe SnakeString) = ExternalOutMsgBody;
(SnakeString probably gonna be added to tlbc soon, yet it's on this branch. your SnakeData type is not defined anyway)
chore: update external-out schema chore: replace TBDs
I have added this into the standard, however I am now being told by multiple members of the community that this is not a good addition. I invite the community to discuss this and/or vote on whether this feature should be removed (specifically, these lines https://github.com/ton-blockchain/TEPs/pull/526/files#diff-837a6387eab3bd4d428b9d6a741cf8f2e14dd62adee63ab40c0eada0c73c37a4R56-R58). Please leave a 👍 reaction to vote for REMOVAL of the feature, and 👎 to vote to LEAVE/INCLUDE the feature in the standard. |
mr-tron
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.